Declutter jwt_spec.rb - #760
Merged
Merged
Conversation
These examples describe what a class must implement to be usable as an algorithm, which is the JWT::JWA::SigningAlgorithm contract, not the JWT.encode/JWT.decode surface that jwt_spec is supposed to cover. Pure move: the block depended only on the shared payload let, and the full suite still reports the same 1246 example descriptions.
The verify_iat block re-tested what claims/issued_at_spec already covers at the unit level: a float iat, an integer iat, one second of clock drift, and a leeway covering that drift. The only thing jwt_spec can say that the claim spec cannot is that the decode options reach the verifier, so it now asserts that and nothing else, including the default of leaving iat unverified, which nothing covered before.
The fixture was a 29 entry hash mixing symbol and string keys, holding everything from an HMAC secret to hardcoded tokens to key fixtures, with six '' placeholders that existed only so before(:each) could overwrite them by mutating a memoized let. Each value now lives where it is used. The HMAC and RSA loops iterate a map of algorithm to expected token rather than indexing the fixture by the loop variable; the ECDSA and PSS loops build their token in a let; the four malformed token strings are inline at their single use each. One value was doing something other than it looked: a keyfinder example resolved its key by using the decoded payload as an index into the shared hash, and the multi key context passed the hardcoded HS256 token string as an HMAC secret. Both now say what they mean. Same 118 examples, and the whole suite reports identical example descriptions before and after.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
First pass at making
spec/jwt/jwt_spec.rbabout one thing: theJWT.encode/JWT.decodepublic surface. 951 lines and 130 examples down to 785 and 118, with no coverage lost.Move the custom algorithm contract out. ~125 lines describing what a class must implement to be usable as an algorithm — that is the
JWT::JWA::SigningAlgorithmcontract, not the encode/decode surface. Nowspec/jwt/jwa/signing_algorithm_spec.rb, mirroring the lib layout. The block depended only on the sharedpayloadlet, so this is a pure move: the suite reports the same 1246 example descriptions before and after, the only diff being the group header.Trim the duplicated iat semantics. The
verify_iatblock re-tested a floatiat, an integeriat, one second of clock drift, and a leeway covering that drift — all four already covered against the claim itself inclaims/issued_at_spec.rb. The only thing this file can say that the claim spec cannot is that the decode options reach the verifier, so that is what it now asserts, plus the default of leavingiatunverified, which nothing covered before.Drop an unused
letin the issuer claim context.Dismantle the
datafixture. A 29 entry hash mixing symbol and string keys, holding an HMAC secret, hardcoded tokens and key fixtures all together, plus six''placeholders that existed only sobefore(:each)could overwrite them by mutating a memoizedlet. Each value now lives where it is used: the HMAC and RSA loops iterate a map of algorithm to expected token instead of indexing the fixture by the loop variable, the ECDSA and PSS loops build their token in alet, and the four malformed token strings are inline at their single use each.Two values were not doing what they appeared to. A keyfinder example resolved its key by using the decoded payload as an index into the shared hash, and the multi-key context passed the hardcoded HS256 token string as an HMAC secret. Both now say what they mean.
What did not move, and why
I had planned to pull the x5c block into
x5c_key_finder_spec.rb, but it doubles outJWT::X5cKeyFinderentirely — it tests thatJWT.decoderoutes thex5coption to the finder, which is squarely the decode surface. Moving it into a spec whose subject it mocks would have made things worse.The same reasoning applies more broadly than I first expected: nearly everything left goes through
JWT.encode/JWT.decodeand legitimately belongs here. The remaining clutter is internal — chiefly the 29-entrydatafixture hash with mixed symbol and string keys, six''placeholders that exist only to be overwritten bybefore(:each), and the mutation of a memoizedlet. That is worth a follow-up, but it is a different change from this one.Verification
dataremoval verified the same way: identical example descriptions across the whole suite before and after, so nothing was added, removed or renamed.--dry-run --format docoutput: identical example descriptions, 1246 before and after.Checklist
Before the PR can be merged be sure the following are checked: